feat: Update Sana Canvas Buttons#4000
Open
josh-bagwell wants to merge 47 commits into
Open
Conversation
… feat/update-buttons-sana-canvas-26-06
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates shared button styling tokens, adds inverse DeleteButton support with story and docs coverage, changes ChangesButton styling and inverse DeleteButton
Upgrade guide documentation
Segmented control sizing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/react/button/lib/DeleteButton.tsx`:
- Around line 14-19: The PR objective specifies a `secondary` variant for
DeleteButton, but the current implementation uses `inverse` across all files,
creating a mismatch between intent and implementation. Align all layers to use
the intended variant name consistently: in
modules/react/button/lib/DeleteButton.tsx lines 14-19, change the
DeleteButtonProps.variant type union from 'inverse' to the intended variant
(e.g., 'secondary'); in modules/react/button/lib/DeleteButton.tsx lines 80-119,
update the modifier block logic to use the intended variant key instead of
'inverse'; in modules/react/button/stories/button/examples/DeleteInverse.tsx
lines 15-24, rename the example component to reflect the intended variant name;
in modules/react/button/stories/button/Button.stories.tsx lines 6-7, update the
import statement to use the renamed example component; in
modules/react/button/stories/button/Button.stories.tsx lines 46-48, update both
the story export name and render target to reference the intended variant; in
modules/react/button/stories/button/Button.mdx lines 12-13, update the import to
reference the renamed example; in modules/react/button/stories/button/Button.mdx
lines 86-90, update all prose references and example blocks to use the intended
variant guidance; and in
modules/react/button/stories/visual-testing/DeleteButton.stories.tsx lines
30-33, update the permutation values and labels to use the intended variant
contract throughout.
- Line 142: The deleteButtonStencil call on line 142 in DeleteButton.tsx is
missing the grow parameter, while PrimaryButton and SecondaryButton both pass it
through to their respective stencil calls. Since deleteButtonStencil extends
buttonStencil which defines the grow modifier, omitting it causes the grow
styling to be silently dropped. Add the grow parameter to the
deleteButtonStencil function call on line 142, passing it alongside the variant
and size parameters to maintain consistency with the other button components.
In `@modules/react/button/lib/SecondaryButton.tsx`:
- Around line 27-33: The SecondaryButton's default state at lines 27-33 uses a
transparent border which relies too heavily on the hover state's border color
change for visual differentiation, creating insufficient perceivable change for
low-vision users. Enhance the default state affordance by either adding a
non-transparent subtle border color, introducing a subtle background shift, or
applying another non-color visual cue to the default state styling. Ensure the
hover state styling (lines 39-43) maintains sufficient contrast and visual
distinction from the enhanced default state. After making these changes,
validate the contrast ratios and state distinguishability during the visual
accessibility review to confirm adequate perceivable differentiation exists
between default and hover states.
In `@modules/react/button/lib/TertiaryButton.tsx`:
- Around line 48-57: The TertiaryButton component in the hover and active state
blocks still includes textDecoration: 'underline', which conflicts with the v16
behavior change objective to remove underlines in those states. Remove the
textDecoration: 'underline' line from both the '&:hover, &.hover' block and the
'&:active, &.active' block to align with the intended styling update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a786b8ad-b7b5-4af3-821e-6612dabc325d
📒 Files selected for processing (8)
modules/react/button/lib/BaseButton.tsxmodules/react/button/lib/DeleteButton.tsxmodules/react/button/lib/SecondaryButton.tsxmodules/react/button/lib/TertiaryButton.tsxmodules/react/button/stories/button/Button.mdxmodules/react/button/stories/button/Button.stories.tsxmodules/react/button/stories/button/examples/DeleteInverse.tsxmodules/react/button/stories/visual-testing/DeleteButton.stories.tsx
Comment on lines
+14
to
+19
| export interface DeleteButtonProps extends ButtonProps { | ||
| /** | ||
| * Variant has an option for `inverse` which will inverse the styling | ||
| */ | ||
| variant?: 'inverse'; | ||
| } |
There was a problem hiding this comment.
Variant contract appears misaligned with the stated PR objective (secondary vs inverse).
The PR objective states DeleteButton should add a secondary variant, but these changes introduce inverse across API, styling, docs, stories, and visual permutations. Please confirm intended product contract, then align all layers consistently.
modules/react/button/lib/DeleteButton.tsx#L14-L19: UpdateDeleteButtonProps.variantunion to the intended variant(s).modules/react/button/lib/DeleteButton.tsx#L80-L119: Rename/rework modifier block to the intended variant key and token behavior.modules/react/button/stories/button/examples/DeleteInverse.tsx#L15-L24: Rename/replace example component to match the intended variant.modules/react/button/stories/button/Button.stories.tsx#L6-L7: Update import to the intended example name.modules/react/button/stories/button/Button.stories.tsx#L46-L48: Update exported story name/render target to intended variant.modules/react/button/stories/button/Button.mdx#L12-L13: Update MDX import for intended variant example.modules/react/button/stories/button/Button.mdx#L86-L90: Update prose/example block to intended variant guidance.modules/react/button/stories/visual-testing/DeleteButton.stories.tsx#L30-L33: Update permutation values/labels to intended variant contract.
📍 Affects 5 files
modules/react/button/lib/DeleteButton.tsx#L14-L19(this comment)modules/react/button/lib/DeleteButton.tsx#L80-L119modules/react/button/stories/button/examples/DeleteInverse.tsx#L15-L24modules/react/button/stories/button/Button.stories.tsx#L6-L7modules/react/button/stories/button/Button.stories.tsx#L46-L48modules/react/button/stories/button/Button.mdx#L12-L13modules/react/button/stories/button/Button.mdx#L86-L90modules/react/button/stories/visual-testing/DeleteButton.stories.tsx#L30-L33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/react/button/lib/DeleteButton.tsx` around lines 14 - 19, The PR
objective specifies a `secondary` variant for DeleteButton, but the current
implementation uses `inverse` across all files, creating a mismatch between
intent and implementation. Align all layers to use the intended variant name
consistently: in modules/react/button/lib/DeleteButton.tsx lines 14-19, change
the DeleteButtonProps.variant type union from 'inverse' to the intended variant
(e.g., 'secondary'); in modules/react/button/lib/DeleteButton.tsx lines 80-119,
update the modifier block logic to use the intended variant key instead of
'inverse'; in modules/react/button/stories/button/examples/DeleteInverse.tsx
lines 15-24, rename the example component to reflect the intended variant name;
in modules/react/button/stories/button/Button.stories.tsx lines 6-7, update the
import statement to use the renamed example component; in
modules/react/button/stories/button/Button.stories.tsx lines 46-48, update both
the story export name and render target to reference the intended variant; in
modules/react/button/stories/button/Button.mdx lines 12-13, update the import to
reference the renamed example; in modules/react/button/stories/button/Button.mdx
lines 86-90, update all prose references and example blocks to use the intended
variant guidance; and in
modules/react/button/stories/visual-testing/DeleteButton.stories.tsx lines
30-33, update the permutation values and labels to use the intended variant
contract throughout.
| grow={grow} | ||
| iconPosition={iconPosition} | ||
| cs={[deleteButtonStencil({size, iconPosition}), cs]} | ||
| cs={[deleteButtonStencil({variant, size, iconPosition}), cs]} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether `grow` is a stencil modifier in button base and compare call-site parity.
set -euo pipefail
echo "== Find grow-related stencil modifier definitions =="
rg -n -C3 '\bgrow\b' modules/react/button/lib/BaseButton.tsx modules/react/button/lib/Button.tsx
echo
echo "== Compare stencil invocation patterns across button components =="
rg -n -C2 'Stencil\(\{[^}]*grow|deleteButtonStencil\(\{|secondaryButtonStencil\(\{|tertiaryButtonStencil\(\{' modules/react/button/libRepository: Workday/canvas-kit
Length of output: 3625
🏁 Script executed:
# Check the deleteButtonStencil definition and its modifier structure
rg -n -A30 'export.*deleteButtonStencil|const deleteButtonStencil' modules/react/button/lib/DeleteButton.tsx | head -50Repository: Workday/canvas-kit
Length of output: 1464
Pass grow into deleteButtonStencil to keep variant parity with other buttons.
On line 142, DeleteButton omits grow in the stencil call while PrimaryButton and SecondaryButton pass it through. Since deleteButtonStencil extends buttonStencil (which defines the grow modifier), this inconsistency silently drops grow styling for DeleteButton.
Suggested fix
- cs={[deleteButtonStencil({variant, size, iconPosition}), cs]}
+ cs={[deleteButtonStencil({variant, size, iconPosition, grow}), cs]}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/react/button/lib/DeleteButton.tsx` at line 142, The
deleteButtonStencil call on line 142 in DeleteButton.tsx is missing the grow
parameter, while PrimaryButton and SecondaryButton both pass it through to their
respective stencil calls. Since deleteButtonStencil extends buttonStencil which
defines the grow modifier, omitting it causes the grow styling to be silently
dropped. Add the grow parameter to the deleteButtonStencil function call on line
142, passing it alongside the variant and size parameters to maintain
consistency with the other button components.
josh-bagwell
changed the title
Workday/canvas-kit
 |
||||||||||||||||||||||||||||||||||||||||
| Project |
Workday/canvas-kit
|
| Branch Review |
feat/update-buttons-sana-canvas-26-06
|
| Run status |
|
| Run duration | 02m 31s |
| Commit |
|
| Committer | Josh |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
17
|
 |
0
|
 |
809
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
19.51%
|
|
|---|---|
|
|
1541
|
|
|
371
|
Accessibility
99.41%
|
|
|---|---|
|
|
5 critical
5 serious
0 moderate
2 minor
|
|
|
72
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/react/button/lib/TertiaryButton.tsx (1)
204-214:⚠️ Potential issue | 🟡 Minor
iconPositionis not derived before being passed to the stencil, causing compound modifiers to not apply.When a consumer renders
<TertiaryButton icon={someIcon}>Label</TertiaryButton>without an expliciticonPosition, the undefined value is passed directly totertiaryButtonStencil. Although theButtoncomponent internally derivesbaseIconPosition = 'start'(for icon + text), that derived value is never used by the stencil, which expects explicit'start','end', or'only'values to match its compound modifiers (lines 98-179).The base
ButtonStencilhas identical compound modifier patterns but with different padding values. SincetertiaryButtonStencilpadding does not apply wheniconPositionis undefined, buttons with icon and label will use only the base button padding (e.g.,md/smfor medium size with 'start') instead of the tertiary-specific padding (e.g.,xs/sm). This creates a visual inconsistency.Derive
baseIconPositioninTertiaryButtonbefore passing it to the stencil, matching the logic inButton.tsx(lines 50-56):const baseIconPosition = iconPosition ? iconPosition : icon ? children ? 'start' : 'only' : undefined; // Then pass baseIconPosition to stencil instead of iconPosition🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/react/button/lib/TertiaryButton.tsx` around lines 204 - 214, The iconPosition parameter is being passed directly to tertiaryButtonStencil without deriving its value first, causing compound modifiers to not apply when iconPosition is undefined. Derive a baseIconPosition variable in TertiaryButton using the same logic from Button.tsx (lines 50-56): check if iconPosition is explicitly provided, otherwise check if icon exists and whether children exist to determine if the position should be 'start' (icon with text), 'only' (icon without text), or undefined. Then pass this derived baseIconPosition to tertiaryButtonStencil instead of the raw iconPosition parameter in the stencil call around line 204-214.
🧹 Nitpick comments (1)
modules/react/button/lib/TertiaryButton.tsx (1)
3-3: 💤 Low valueUnused import:
px2remis not referenced in this file.The
px2remutility is imported but not used anywhere in the component. Consider removing it to keep imports clean.-import {createStencil, cssVar, px2rem} from '`@workday/canvas-kit-styling`'; +import {createStencil, cssVar} from '`@workday/canvas-kit-styling`';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/react/button/lib/TertiaryButton.tsx` at line 3, Remove the unused `px2rem` import from the import statement at the top of the TertiaryButton.tsx file. The import statement should only include `createStencil` and `cssVar` from '`@workday/canvas-kit-styling`', since `px2rem` is not used anywhere in the component.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@modules/react/button/lib/TertiaryButton.tsx`:
- Around line 204-214: The iconPosition parameter is being passed directly to
tertiaryButtonStencil without deriving its value first, causing compound
modifiers to not apply when iconPosition is undefined. Derive a baseIconPosition
variable in TertiaryButton using the same logic from Button.tsx (lines 50-56):
check if iconPosition is explicitly provided, otherwise check if icon exists and
whether children exist to determine if the position should be 'start' (icon with
text), 'only' (icon without text), or undefined. Then pass this derived
baseIconPosition to tertiaryButtonStencil instead of the raw iconPosition
parameter in the stencil call around line 204-214.
---
Nitpick comments:
In `@modules/react/button/lib/TertiaryButton.tsx`:
- Line 3: Remove the unused `px2rem` import from the import statement at the top
of the TertiaryButton.tsx file. The import statement should only include
`createStencil` and `cssVar` from '`@workday/canvas-kit-styling`', since `px2rem`
is not used anywhere in the component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0540207b-8d15-4719-9b1d-15792a8765c5
📒 Files selected for processing (3)
modules/react/button/lib/BaseButton.tsxmodules/react/button/lib/SecondaryButton.tsxmodules/react/button/lib/TertiaryButton.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/react/button/lib/SecondaryButton.tsx
- modules/react/button/lib/BaseButton.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
modules/react/button/lib/SecondaryButton.tsx (1)
3-3: ⚡ Quick winRemove unused
colorSpaceimport.The
colorSpaceimport is no longer used after removing thecolorSpace.darken()calls from hover/active states.♻️ Suggested fix
-import {colorSpace, createStencil, cssVar} from '`@workday/canvas-kit-styling`'; +import {createStencil, cssVar} from '`@workday/canvas-kit-styling`';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/react/button/lib/SecondaryButton.tsx` at line 3, Remove the unused `colorSpace` import from the import statement in SecondaryButton.tsx. The `colorSpace` module is no longer referenced in the code after the `colorSpace.darken()` calls were removed from the hover and active states. Keep the other imports `createStencil` and `cssVar` which are still in use. Simply delete `colorSpace` from the destructured import list from '`@workday/canvas-kit-styling`'.modules/react/button/lib/TertiaryButton.tsx (1)
1-1: ⚡ Quick winRemove unused
focusRingimport.The
focusRingimport is no longer used after refactoring focus styles to use explicitboxShadowInner/boxShadowOutervalues.♻️ Suggested fix
-import {createComponent, focusRing} from '`@workday/canvas-kit-react/common`'; +import {createComponent} from '`@workday/canvas-kit-react/common`';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/react/button/lib/TertiaryButton.tsx` at line 1, Remove the unused `focusRing` import from the import statement at the top of TertiaryButton.tsx. The `focusRing` symbol is no longer needed since the focus styles have been refactored to use explicit `boxShadowInner` and `boxShadowOuter` values instead. Keep the `createComponent` import and remove only `focusRing` from the destructured imports coming from '`@workday/canvas-kit-react/common`'.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@modules/react/button/lib/SecondaryButton.tsx`:
- Line 3: Remove the unused `colorSpace` import from the import statement in
SecondaryButton.tsx. The `colorSpace` module is no longer referenced in the code
after the `colorSpace.darken()` calls were removed from the hover and active
states. Keep the other imports `createStencil` and `cssVar` which are still in
use. Simply delete `colorSpace` from the destructured import list from
'`@workday/canvas-kit-styling`'.
In `@modules/react/button/lib/TertiaryButton.tsx`:
- Line 1: Remove the unused `focusRing` import from the import statement at the
top of TertiaryButton.tsx. The `focusRing` symbol is no longer needed since the
focus styles have been refactored to use explicit `boxShadowInner` and
`boxShadowOuter` values instead. Keep the `createComponent` import and remove
only `focusRing` from the destructured imports coming from
'`@workday/canvas-kit-react/common`'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e259c913-e45b-4c79-a787-d8e8f4692428
📒 Files selected for processing (6)
modules/react/button/lib/BaseButton.tsxmodules/react/button/lib/DeleteButton.tsxmodules/react/button/lib/PrimaryButton.tsxmodules/react/button/lib/SecondaryButton.tsxmodules/react/button/lib/TertiaryButton.tsxmodules/styling/lib/colorSpace.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/react/button/lib/BaseButton.tsx
- modules/react/button/lib/DeleteButton.tsx
… feat/update-buttons-sana-canvas-26-06
… feat/update-buttons-sana-canvas-26-06
…sh-bagwell/canvas-kit into feat/update-buttons-sana-canvas-26-06
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/styling/spec/colorSpace.spec.ts (1)
29-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit
surfaceType: 'inverse'assertions for hover/pressed.Line 29 and Line 54 currently validate only the
defaultsurface branch. SincecolorSpacenow branches onsurfaceType, add inverse-path expectations too to prevent token-regression gaps.Also applies to: 54-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/styling/spec/colorSpace.spec.ts` around lines 29 - 41, The hover and pressed specs in colorSpace.spec.ts only cover the default surface branch, so add explicit assertions for the inverse surface path as well. Update the existing colorSpace.hover and colorSpace.pressed test cases to include a `surfaceType: 'inverse'` variant and verify the expected token strings for that branch, using the same `colorSpace` helper and token placeholders already in the spec.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/react/button/lib/PrimaryButton.tsx`:
- Around line 88-91: The hover and active state colorSpace calls in
PrimaryButton are using the default surface overlay path instead of the inverse
variant, which can produce incorrect overlays for inverse buttons. Update the
affected colorSpace.hover and colorSpace.active usages in PrimaryButton to pass
surfaceType: 'inverse' alongside the existing color and colorType arguments so
the inverse state styling resolves correctly.
---
Nitpick comments:
In `@modules/styling/spec/colorSpace.spec.ts`:
- Around line 29-41: The hover and pressed specs in colorSpace.spec.ts only
cover the default surface branch, so add explicit assertions for the inverse
surface path as well. Update the existing colorSpace.hover and
colorSpace.pressed test cases to include a `surfaceType: 'inverse'` variant and
verify the expected token strings for that branch, using the same `colorSpace`
helper and token placeholders already in the spec.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fe106b54-766a-4ed4-bc99-4825bf6a70d3
📒 Files selected for processing (7)
modules/react/button/lib/BaseButton.tsxmodules/react/button/lib/DeleteButton.tsxmodules/react/button/lib/PrimaryButton.tsxmodules/react/button/lib/SecondaryButton.tsxmodules/react/button/lib/TertiaryButton.tsxmodules/styling/lib/colorSpace.tsxmodules/styling/spec/colorSpace.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- modules/styling/lib/colorSpace.tsx
- modules/react/button/lib/DeleteButton.tsx
- modules/react/button/lib/SecondaryButton.tsx
- modules/react/button/lib/TertiaryButton.tsx
Comment on lines
+88
to
+91
| [buttonStencil.vars.background]: colorSpace.hover({ | ||
| color: cssVar(brand.action.lightest, system.legacy.color.surface.inverse), | ||
| mixinColor: system.legacy.color.accent.overlay.mixin, | ||
| mixinValue: system.legacy.opacity.accent.hover, | ||
| fallback: cssVar(brand.action.lightest, system.legacy.color.surface.inverse), | ||
| colorType: 'surface', |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass surfaceType: 'inverse' in inverse hover/active colorSpace calls.
Line 88 and Line 110 currently use the default surface overlay branch, so inverse buttons can resolve incorrect state overlays.
Suggested fix
'&:hover, &.hover': {
[buttonStencil.vars.background]: colorSpace.hover({
color: cssVar(brand.action.lightest, system.legacy.color.surface.inverse),
fallback: cssVar(brand.action.lightest, system.legacy.color.surface.inverse),
colorType: 'surface',
+ surfaceType: 'inverse',
}), '&:active, &.active': {
[buttonStencil.vars.background]: colorSpace.pressed({
color: cssVar(brand.action.lightest, system.legacy.color.surface.inverse),
fallback: cssVar(brand.action.lightest, system.legacy.color.surface.inverse),
colorType: 'surface',
+ surfaceType: 'inverse',
}),Also applies to: 110-113
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/react/button/lib/PrimaryButton.tsx` around lines 88 - 91, The hover
and active state colorSpace calls in PrimaryButton are using the default surface
overlay path instead of the inverse variant, which can produce incorrect
overlays for inverse buttons. Update the affected colorSpace.hover and
colorSpace.active usages in PrimaryButton to pass surfaceType: 'inverse'
alongside the existing color and colorType arguments so the inverse state
styling resolves correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/styling/lib/colorSpace.tsx (1)
83-98: 🎯 Functional Correctness | 🔴 CriticalCorrect implementation and tests must be updated to address mismatch.
The
InteractiveStatePropsinterface correctly removedsurfaceType, but the function implementation inmodules/styling/lib/colorSpace.tsxnow resolves tokens tosystem.legacy.color[colorType].overlay.mixin(a single generic color) rather than the specificsurfaceType-suffixed tokens expected by tests.This results in:
- Token Regression: The CSS generated no longer differentiates between
defaultandinversesurface variants, collapsing them to--cnvs-sys-color-surface-overlay-mixin.- Failing Tests:
modules/styling/spec/colorSpace.spec.ts(lines 33-39, 58-64) explicitly passes the undefinedsurfaceTypeproperty and expects the old token structure (e.g.,--cnvs-sys-color-surface-overlay-hover-default).- Type Safety: Callers passing
surfaceTypewill encounter TypeScript errors ifInteractiveStatePropsremains the interface.The implementation logic should either be updated to support the specific token suffixes if distinct behavior is required, or the tests/documentation must be updated to remove
surfaceTypearguments and update expected output strings to match the genericoverlay-mixintoken being used.Comparison
Current Implementation Output:
color-mix(in srgb, var(--cnvs-sys-color-brand-accent-primary, var(--cnvs-brand-primary-base)) , var(--cnvs-sys-color-surface-overlay-mixin, var(black)) calc(var(--cnvs-sys-opacity-surface-hover, 0) * 100%))Test File Expectation (lines 38, 63):
// Expects distinct hover vs pressed suffixes and default suffix var(--cnvs-sys-color-surface-overlay-hover-default, black)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/styling/lib/colorSpace.tsx` around lines 83 - 98, The interactive color token resolution in colorSpace.tsx no longer matches the spec expectations because it now uses the generic surface overlay mixin token instead of surfaceType-specific suffixes. Update the implementation around the InteractiveStateProps-driven color mix logic to either restore distinct default/inverse surface token handling or align the tests and docs with the new generic overlay-mixin behavior. Also update modules/styling/spec/colorSpace.spec.ts to remove surfaceType usage and assert the new resolved token strings consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/styling/lib/colorSpace.tsx`:
- Around line 113-120: The hover and pressed state color helpers are both using
the wrong overlay token path, so update the state-specific mixin selection in
the interactive color helpers in colorSpace.tsx: have hover resolve from
system.legacy.color[colorType].overlay.hover and pressed resolve from
system.legacy.color[colorType].overlay.pressed instead of overlay.mixin, keeping
the existing darken call structure intact so the generated CSS variables match
the spec and the hover/pressed token names remain distinct.
---
Outside diff comments:
In `@modules/styling/lib/colorSpace.tsx`:
- Around line 83-98: The interactive color token resolution in colorSpace.tsx no
longer matches the spec expectations because it now uses the generic surface
overlay mixin token instead of surfaceType-specific suffixes. Update the
implementation around the InteractiveStateProps-driven color mix logic to either
restore distinct default/inverse surface token handling or align the tests and
docs with the new generic overlay-mixin behavior. Also update
modules/styling/spec/colorSpace.spec.ts to remove surfaceType usage and assert
the new resolved token strings consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 13bafb38-215e-4b5a-a075-c0c9ffab587f
📒 Files selected for processing (1)
modules/styling/lib/colorSpace.tsx
sheelah
reviewed
Jun 25, 2026
sheelah
reviewed
Jun 25, 2026
sheelah
reviewed
Jun 25, 2026
| // Hover Styles | ||
| '&:hover, &.hover': { | ||
| [buttonStencil.vars.background]: colorSpace.darken({ | ||
| [buttonStencil.vars.background]: colorSpace.hover({ |
Contributor
There was a problem hiding this comment.
Related to PrimaryButton, wondering if the ThemeOverrides story content is still fully accurate and worth keeping. I noticed the "Override Primary Color Via Canvas Provider" story renders PrimaryButtons that also don't show a hover color.
Contributor
Author
There was a problem hiding this comment.
Yeah, this is an issue with removing the data-theme from tokens.
sheelah
reviewed
Jun 25, 2026
sheelah
left a comment
sheelah
left a comment
Contributor
There was a problem hiding this comment.
Thanks for tackling this tricky one! Left a few comments inline.
sheelah
reviewed
Jun 25, 2026
| { | ||
| variant: [ | ||
| {value: undefined, label: ''}, | ||
| {value: 'inverse', label: 'Inverse'}, |
Contributor
There was a problem hiding this comment.
Do we want to wrap the inverse variant in a container with a dark background, for consistency with the other button components' inverse variant visual tests?
Contributor
Author
There was a problem hiding this comment.
Yeah I can update this.
Contributor
Author
There was a problem hiding this comment.
I did but, I'm not sure where this variant should exist?
I feel like this is now an accessibility issue with a dark background.
Contributor
There was a problem hiding this comment.
Yeah agreed -- think we should bring this up with design.
RayRedGoose
reviewed
Jun 26, 2026
RayRedGoose
left a comment
RayRedGoose
left a comment
Contributor
There was a problem hiding this comment.
Looking good. Left some small notes, but it's a good candidate to get merge once a11y review is done 👍
Comment on lines
+7
to
+10
| ## Why You Should Upgrade | ||
|
|
||
| Soon to be... | ||
|
|
RayRedGoose
reviewed
Jun 30, 2026
|
|
||
| ### Buttons | ||
|
|
||
| #### Delete Button Inverse Variant |
Contributor
There was a problem hiding this comment.
Do we change to another version name?
Contributor
Author
There was a problem hiding this comment.
Yeah I just pushed an update for this.
It's now Outline.
<DeleteButton variant="outline">Delete</DeleteButton>… feat/update-buttons-sana-canvas-26-06
…sh-bagwell/canvas-kit into feat/update-buttons-sana-canvas-26-06
RayRedGoose
reviewed
Jun 30, 2026
… feat/update-buttons-sana-canvas-26-06
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes: #3976
Release Category
Components
Release Note
This updates our Button styling to introduce Sana Canvas.
BREAKING CHANGES
Visual Changes
Where Should the Reviewer Start?
/modules/react/buttonSummary by CodeRabbit
Summary by CodeRabbit
New Features
DeleteButton, including Storybook/MDX examples and visual-testing state permutations.DeleteButtoninverse variant.Style
colorSpace.hover()/colorSpace.pressed()to derive values from legacy tokens.Bug Fixes
SegmentedControl.Itemlayout by honoring content min-width (min-content).Tests
colorSpacehover/pressed coverage forcolorType(accentvssurface).